-
Notifications
You must be signed in to change notification settings - Fork 4.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds edit and view buttons #1905
Conversation
@takluyver It's working for me. However, on Chrome, the user must enable pop-ups for the domain, otherwise only the first file in the selection will be opened and a pop-up warning will display in the address bar. |
I think that's what Min expected on #1755. I'm inclined to make it a single-item action so that we're not dealing with browser pop-up blockers. |
@gnestor I think this is still useful. I'd just set the limit at 1, so that it doesn't run into pop-up blockers. |
@@ -595,6 +597,20 @@ define([ | |||
$('.delete-button').css('display', 'none'); | |||
} | |||
|
|||
// View is visible when an item is renderable or downloadable | |||
if (selected.length > 0 && !has_directory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
selected.length === 1 here, being what I mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say let the popup blockers do their job, and let the user manage that, rather than making it harder to open many files at once. When I am grading (not with nbgrader) selecting all, and View saves me a lot of time. Of course, you can only do this with class sizes less than about 25, but that is where I live anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsblank I'm just curious: How do you use "View" and "Edit" for grading? I assumed "View" would open the notebook, but it opens the raw JSON file in the browser and "Edit" opens the JSON file in a text editor...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes, I just want to open the notebooks as notebooks (with kernel), be able to run, view, and print them. That was my intention by adding these buttons for the group selection operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, then I believe the expected behavior for "View" is to open notebooks as a notebook vs. raw file (see my screencap below for an example). Given that, we can remove "View" entirely because viewing and editing any file other than a notebook is the same (e.g. open it in a text editor) and editing a notebook is to open it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about HTML files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point! HTML files seem like the one file type that should provide a "View" button. I'll try that now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great... that looks useful! What about all other types that one can view in the browser, such image files (jpg, gif)? Could there be a way to register editors for particular type of files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the primary purpose of this PR is to allow "View" and "Edit" on multiple files at once. If we only allowed these button for single file selections, then there would be no need for "Edit" as it has the same effect of clicking on a file, and we are realizing that "View" is only needed for HTML and image files...so if we remove this for multiple files, we are left with only "View" for a handful of file types.
Sweet ! +1 |
Nice! |
Looking forward to have an occasion to use that ! |
@@ -595,6 +597,22 @@ define([ | |||
$('.delete-button').css('display', 'none'); | |||
} | |||
|
|||
// View is visible when an item is renderable or downloadable | |||
if (selected.length > 0 && !has_directory && selected.every(function(el) { | |||
return el.path.match(/html?|jpe?g|png|gif|tiff?|svg|bmp|ico|pdf|doc|xls/); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsblank I updated the "View" condition to a regex of common browser-renderable file types. I could create an API method on NotebookList to register additional file types, but it would probably be easier to just decide on them now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gnestor Thank you... this is very good for this version. Later, when there is a full API, I agree that being able to register mime-types and specific viewers/editors would be quote useful. This is great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're welcome! If you take a look at JupyterLab and specifically https://github.com/jupyterlab/jupyterlab_json, you will see that lab allows for extensions to both render output by mime type and render files by extension, so there is a lot opportunity to create custom file handlers.
I think it might be a bit surprising for new users that 'View' and 'Edit' on a notebook both show you the raw JSON. I understand that that fits in with editing other text-based file types, but I would naively expect 'edit' on a notebook to be equivalent to opening it normally, and 'view' to be equivalent to the nbconvert-based 'print preview' we offer from the file menu. How hard would it be to make the buttons say something like 'View raw' and 'Edit raw' if you have a notebook selected? |
@takluyver I've revised so that "View" is only visible for HTML, image, and PDF files and "Edit" will open notebooks as notebooks and all other files in the text editor. |
One more use case to consider: What if you wanted to look at or edit the JSON of a notebook file? There is currently no way to open a notebook in a text editor. Do we want to accommodate that with an "Edit Raw" or "Edit Metatdata" for selected notebook files? |
I don't think we need a button for that.... Maybe be able to open from editor would be enough? |
Thanks @gnestor, that sounds good to me. I don't think there's a pressing need for 'edit raw'. I'd still probably agree with Min that I'd only show the buttons with a single file selected, and avoid popup blockers altogether. I think the main thing we wanted to fix is that there's no easy way to view an HTML file, because clicking on it opens an editor. I don't feel strongly about this, though. We should think about the security implementations. An HTML file could contain Javascript, which can execute when you 'view' it. Because it's served from the notebook domain, that Javascript can presumably start a kernel and use it to execute code (unless I've overlooked something). That seems a bit unexpected for a 'view' action. Is there any way we can mitigate this? Ping @rgbkrk |
That's exactly correct. If the view needs to be supported it needs to be in a sandboxed iframe (since we don't have access to other subdomains). |
As for showing the buttons when there is only 1 vs. more selected, often times the better option doesn't make itself clear until you try to write the documentation (or teach someone how to use it). If you start describing how to use this, it becomes complicated... buttons showing/not showing because different combinations of types (files, directories) and counts (one, two) are being selected. If you can make an option more flexible and easier to explain, then go with that. |
Aha, so we would use that to disable JS scripts entirely for the HTML you're viewing? I'm going to bump this PR to milestone 5.0 as it seems like a bigger thing to work out, and I'd like to get 4.3 released fairly soon. |
I certainly agree with that. |
It's not that we need to disable JS so much as we want to disable access to the Jupyter REST APIs and websockets. <iframe sandbox="allow-scripts" src="test.html">
</iframe> However, you can disable JS by omitting the <iframe sandbox src="test.html">
</iframe> |
Ah, I see, a sandboxed iframe has a special origin, so any attempt to access the Jupyter REST APIs would be blocked by the cross-origin protection. Thanks, I didn't know about that :-) |
Another security option that is available includes disabling scripts on specific paths with a more stringent content security policy. |
I think that the general 'treat this as a different origin' security measure should do the trick. @gnestor does this make sense? I imagine we will need a separate handler at something like <!-- /viewhtml/foo.html -->
<iframe sandbox="allow-scripts" src="/files/foo.html">
</iframe> And viewing an HTML file would take you to this page instead of the |
@takluyver Makes sense. Thanks! @rgbkrk Could we modify the CSP in lieu of redirecting to a page with an iframe? If so, that seems like a simpler solution... @minrk I see that you created the FileRedirectHandler, how would you go about disallowing HTML files from accessing the Jupyter REST APIs? If we went with iframe approach, would it make sense to create a self.render_template('file.html', source='/files/foo.html') |
I think IFrames are the only way to do this. I'm not sure of the details, though. A properly configured IFrame should prevent access to the API via existing CORS, etc. |
@minrk Ok, then we'll take that approach. Would you recommend creating a template and doing something like this in self.write(self.render_template('file.html', source='/files/foo.html')) |
Yeah, a template for the iframe page sounds quite sensible. |
Ping - are we ready to implement this with the iframe for 5.0? |
@takluyver Yes, I will do this next. Thanks for the reminder 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to see some tests of the functionality provided here before merging it. Adding new features without tests makes for future sad panda developers and users.
Update:
I would appreciate any feedback regarding tests and UX before merging. |
} | ||
|
||
// Edit is visible when an item is editable | ||
if (selected.length > 0 && !has_directory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to whitelist/blacklist files for editability? E.g. you can't open a png in the text editor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use a regex pattern like above to include/exclude specific extensions. I just opened a PNG in the text editor and it's pretty useless and harmless, so I'm neutral.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was what I was thinking. :-) I'd probably go for a blacklist. Maybe there are too many types to be worth trying to list them all, though. Do we have mimetypes here? Maybe we could block edit on image/*
, with an exception for image/svg+xml
.
* Edit notebook opens a notebook, Edit any other file type open it in a text editor * View only available for HTML files to disambiguate between “viewing” a notebook or text file (which is the same as editing it) and viewing an HTML file
OK, let's land this, we can refine things later. |
Continuation of #1755
closes #1752